-
-
Notifications
You must be signed in to change notification settings - Fork 24
feat(cache): allow to store metadata associated with response #120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
I could see this being useful, though I think it might be worth wrapping the value in an Option |
80b589a to
13c8df0
Compare
|
I wrapped the metadata in an Option, i agree it's better in term of DX, also updated underlying library to the new process_response API. At the moment passing metadata is only possible if we use directly the We could propose a So end-user could add this type to its response extension. What would be better it's automatically pass all extensions values of the response into the metadata, but not sure if this possible. |
I could have sworn I responded to this but I guess it never sent. I think some integration at the client end makes sense. With reqwest there's this https://docs.rs/reqwest-middleware/latest/reqwest_middleware/struct.Extension.html, maybe something could be integrated there? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #120 +/- ##
==========================================
- Coverage 93.91% 85.54% -8.37%
==========================================
Files 10 19 +9
Lines 1233 7362 +6129
==========================================
+ Hits 1158 6298 +5140
- Misses 75 1064 +989 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I think we could do something like this: Which works along the same ways as the cache mode function and whatnot |
|
@joelwurtz Added that implementation along with some tests to serve as examples, seems to work pretty well. Curious if you believe this solution would meet the requirements on your end |
|
I think it should work, as i already pass compute logic with the response extension API, we can even remove the metadata argument to the However i don't see how users could retrieve those Metadata when using one of the middleware ? I think it should be automatically added back to the extension of the response, maybe i miss it ?, so he could do : let metadata_as_bytes = response.extensions().get::<HttpCacheMetadata>();I will branch my project on this branch next week to ensure that it works nicely. Thanks a lot for the follow up. |
|
Made a few adjustments and currently the flow looks about like: use http_cache_reqwest::{Cache, CacheMode, HttpCache, HttpCacheOptions};
use http_cache::{CACacheManager, HttpCacheMetadata};
use reqwest::Client;
use reqwest_middleware::ClientBuilder;
use std::sync::Arc;
#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
// Create cache options with a metadata provider
let options = HttpCacheOptions {
metadata_provider: Some(Arc::new(|request_parts, response_parts| {
// Generate metadata based on request/response
// Example: Store the ETag and Last-Modified for later use
let mut metadata = Vec::new();
if let Some(etag) = response_parts.headers.get("etag") {
metadata.extend_from_slice(b"etag:");
metadata.extend_from_slice(etag.as_bytes());
metadata.push(b'\n');
}
if let Some(last_modified) = response_parts.headers.get("last-modified") {
metadata.extend_from_slice(b"last-modified:");
metadata.extend_from_slice(last_modified.as_bytes());
}
if metadata.is_empty() {
None
} else {
Some(metadata)
}
})),
..Default::default()
};
// Build the client with caching
let client = ClientBuilder::new(Client::new())
.with(Cache(HttpCache {
mode: CacheMode::Default,
manager: CACacheManager::default(),
options,
}))
.build();
// Make a request
let response = client
.get("https://api.example.com/data")
.send()
.await?;
// Check if we got cached metadata
if let Some(metadata) = response.extensions().get::<HttpCacheMetadata>() {
let metadata_str = String::from_utf8_lossy(metadata.as_slice());
println!("Cache metadata: {}", metadata_str);
// Parse and use the metadata as needed
for line in metadata_str.lines() {
if let Some(etag) = line.strip_prefix("etag:") {
println!("Cached ETag: {}", etag);
}
if let Some(last_modified) = line.strip_prefix("last-modified:") {
println!("Cached Last-Modified: {}", last_modified);
}
}
} else {
println!("No cache metadata (cache miss or no metadata stored)");
}
// Use the response body as normal
let body = response.text().await?;
println!("Response: {}", body);
Ok(())
} |
Hey,
I think an example illustrate more than a long text, so here is a PR of something we need : associate metadata with a response in cache.
We need this because we have a middleware than runs after the cache which :
We still want to log, even if we cache, so we will extract the last part into an other middleware that will run before the cache, however we don't want to recompute some informations needed as it can be costly and will always be the same for a given cache.
We do think saving this information with the response cache would resolve our need, there may be other use cases where having the possibility to have metadata in the response could be useful (as an exemple if you use response extensions, you may want to store them inside cache).
About the implementation i used a
Vec<u8>so metadata will need to be serialized / deserialized by the user (so there is double serialization involved with the HttpResponse also being serialized)I studied the possibility to have
But it need a lot of refactoring to handle this everywhere and add some complexity since we will need to add many restrictions on this generic type (being Serializable / Deserializable / Send / Sync / ...) i prefer to show this before going there if needed.